Skip to content

vm: fix produceCachedData#5343

Closed
jray319 wants to merge 2 commits into
nodejs:masterfrom
jray319:simple-fix
Closed

vm: fix produceCachedData#5343
jray319 wants to merge 2 commits into
nodejs:masterfrom
jray319:simple-fix

Conversation

@jray319

@jray319 jray319 commented Feb 21, 2016

Copy link
Copy Markdown
Contributor

Fix segmentation faults when compiling the same code with
produceCachedData option. V8 ignores the option when the code is in its
compilation cache and does not return cached data. Added
cachedDataProduced property to v8.Script to denote whether the cached
data is produced successfully.

Fix segmentation faults when compiling the same code with
`produceCachedData` option. V8 ignores the option when the code is in its
compilation cache and does not return cached data. Added
`cachedDataProduced` property to `v8.Script` to denote whether the cached
data is produced successfully.
@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Feb 21, 2016
@mscdex

mscdex commented Feb 21, 2016

Copy link
Copy Markdown
Contributor

/cc @indutny

Comment thread src/node_contextify.cc Outdated
reinterpret_cast<const char*>(cached_data->data),
cached_data->length);
args.This()->Set(env->cached_data_string(), buf.ToLocalChecked());
bool cached_data_produced = (cached_data != nullptr);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need in parens.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. Thanks.

@indutny

indutny commented Feb 21, 2016

Copy link
Copy Markdown
Member

One nit, otherwise looking good. Going to give a try before landing, though.

@indutny

indutny commented Feb 21, 2016

Copy link
Copy Markdown
Member

@indutny

indutny commented Feb 21, 2016

Copy link
Copy Markdown
Member

LGTM

@indutny

indutny commented Feb 21, 2016

Copy link
Copy Markdown
Member

@jasnell this is not in v4.x, please remove the tag.

@MylesBorins

Copy link
Copy Markdown
Contributor

@indutny the watch tag is for us to audit a commit to be backported

@indutny

indutny commented Feb 21, 2016

Copy link
Copy Markdown
Member

@thealphanerd I'm just saying that it should be don't land on v4.x

@MylesBorins

Copy link
Copy Markdown
Contributor

Ahhh... got it 😄

@indutny

indutny commented Feb 21, 2016

Copy link
Copy Markdown
Member

Landed in d5c04c3, thank you!

@indutny indutny closed this Feb 21, 2016
indutny pushed a commit that referenced this pull request Feb 21, 2016
Fix segmentation faults when compiling the same code with
`produceCachedData` option. V8 ignores the option when the code is in
its compilation cache and does not return cached data. Added
`cachedDataProduced` property to `v8.Script` to denote whether the
cached data is produced successfully.

PR-URL: #5343
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@indutny

indutny commented Feb 21, 2016

Copy link
Copy Markdown
Member

Changed tags myself.

@indutny

indutny commented Feb 21, 2016

Copy link
Copy Markdown
Member

Argh, concurrency! :)

@MylesBorins

Copy link
Copy Markdown
Contributor

@indutny would you be able to add a small sentence about why this commit is not an LTS candidate?

@indutny

indutny commented Feb 21, 2016

Copy link
Copy Markdown
Member

Because produceCachedData is a v5.x only thing.

@indutny

indutny commented Feb 21, 2016

Copy link
Copy Markdown
Member

@thealphanerd we may want to reconsider this a bit later, though. I just don't want to make a commitment to supporting this on a LTS level yet. If things will work out - there are no blockers in backporting all of code cache changes to v4.x and releasing them in next minor.

rvagg pushed a commit that referenced this pull request Feb 21, 2016
Fix segmentation faults when compiling the same code with
`produceCachedData` option. V8 ignores the option when the code is in
its compilation cache and does not return cached data. Added
`cachedDataProduced` property to `v8.Script` to denote whether the
cached data is produced successfully.

PR-URL: #5343
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

vm Issues and PRs related to the vm subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants